Skip to content

Adapted the makefiles to the new build system #1232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 5, 2016
Merged

Adapted the makefiles to the new build system #1232

merged 1 commit into from
Aug 5, 2016

Conversation

@sergioamr
Copy link
Contributor

@sergioamr sergioamr commented Aug 1, 2016

  • Zephyr now requires two passes to create the configuration for the cross compiling
  • Added the missing bits required to build a valid new jerryscript minimal configuration

Tested on Arduino101, qemu_x86, qemu_cortex_m3

  • Comment:
    I don't see how to generate a toolchain cmake with the new system in the form
    of cmake\toolchain_zephyr... and use the build.py.

All the parameters are dynamically generated by the Zephyr makefile includes.

JerryScript-DCO-1.0-Signed-off-by: Sergio Martinez sergio.martinez.rodriguez@intel.com

@sergioamr
Copy link
Contributor Author

sergioamr commented Aug 1, 2016

@pfalcon @robertsipka @LaszloLango Please have a look

@LaszloLango LaszloLango added bug Undesired behaviour jerry-port Related to the port API or the default port implementation labels Aug 2, 2016
LaszloLango
LaszloLango reviewed Aug 2, 2016
View reviewed changes
targets/zephyr/Makefile.zephyr Outdated
-DEXTERNAL_BUILD_ENTRY_FILE=$(TARGET_ZEPHYR_SRC_DIR)/jerry-entry.c
-DCMAKE_TOOLCHAIN_FILE=cmake/toolchain_external.cmake \
-DEXTERNAL_BUILD_ENTRY_FILE=$(TARGET_ZEPHYR_SRC_DIR)/jerry-entry.c \
-DFEATURE_SNAPSHOT_EXEC=OFF \
Copy link
Contributor

@LaszloLango LaszloLango Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong indentation

@LaszloLango
Copy link
Contributor

LaszloLango commented Aug 2, 2016

LGTM after the minor style issues are fixed.

robertsipka
robertsipka reviewed Aug 2, 2016
View reviewed changes
targets/zephyr/Makefile.zephyr Outdated
mkdir -p $(OUTPUT)
cmake -B$(INTERM) -H./ \
-DENABLE_LTO=OFF \
-DENABLE_VALGRIND=OFF \
Copy link
Contributor

@robertsipka robertsipka Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It changed its name to FEATURE_VALGRIND

@sergioamr
Copy link
Contributor Author

sergioamr commented Aug 2, 2016

@robertsipka Nice one, thanks for looking at the code and the feedback. I am not sure if i will have the time today but will fix those issues soon.

@sergioamr sergioamr force-pushed the wip/sergio/zephyr_new_build_system branch 3 times, most recently from a2ba8ea to f3f5ccc Compare August 3, 2016 11:17
@sergioamr
Copy link
Contributor Author

sergioamr commented Aug 3, 2016

@robertsipka Still haven't figure out what the problem is with time_t, it seems to be defined as "long" for the arduino_101 inside the SDK, which conflicts with the unsigned for the time calculation.

Before this change i think the definitions from JS were first so it was defined properly somewhere.
If i compile for other architectures the sdk provides the right time_t definition.

Will investigate further.

@sergioamr sergioamr force-pushed the wip/sergio/zephyr_new_build_system branch from f3f5ccc to b6fad8f Compare August 3, 2016 12:52
@pfalcon
Copy link
Contributor

pfalcon commented Aug 3, 2016

@sergioamr : Nice work! Let me however dump my mind with things I know about related matters (I'm on and off these days, so late spotting some things):

  • ~week ago Zephyr update broke building JerryScript for me, there was "stdlib.h not found error"
  • I proceeded to check in-tree samples/static_lib (which uses similar external source compile process) and found it to be broken too.
  • Investigating it, I came up with https://gerrit.zephyrproject.org/r/3521 . As you can see, I actually had to patch Makefile.toolchain.zephyr to let it be included from an external Makefile.
  • Then JrS build system was updated too.
  • Then this patch came in, and I was surprised to see it build, without changes to Makefile.toolchain.zephyr (I didn't look into it, just built).
  • However, while qemu_x86 and qemu_cortex_m3 run ok for me, flashing this to BOARD=frdm_k64f, it doesn't work (just hangs).

So, while this patch is definitely is a good thing, not everything is right yet, though I don't know where issue is. Anyway, that shouldn't be a reason to delay this patch, so as long as JrS maintainers are ok with it, I'm +1 on merging it too.

@robertsipka
Copy link
Contributor

robertsipka commented Aug 4, 2016

LGTM (informally)

@zherczeg
Copy link
Member

zherczeg commented Aug 4, 2016

LGTM

@pfalcon
Copy link
Contributor

pfalcon commented Aug 4, 2016

However, while qemu_x86 and qemu_cortex_m3 run ok for me, flashing this to BOARD=frdm_k64f, it doesn't work (just hangs).

This turned out to be problem with multilib detection for Cortex-M4 (as used on frdm_k64f). It was fixed by Zephyr SDK 0.8.2 release yesterday. So firm +1 from me on this patch.

@LaszloLango
Copy link
Contributor

LaszloLango commented Aug 5, 2016

@sergioamr, please rebase to the current master, so we can land it properly.

@sergioamr
Copy link
Contributor Author

sergioamr commented Aug 5, 2016

@LaszloLango Done

@LaszloLango
Copy link
Contributor

LaszloLango commented Aug 5, 2016

@sergioamr, something went wrong when you rabased the PR. Now you have 7 commits. :)

@sergioamr sergioamr force-pushed the wip/sergio/zephyr_new_build_system branch from db17557 to 03c6d8e Compare August 5, 2016 11:06
@sergioamr
Copy link
Contributor Author

sergioamr commented Aug 5, 2016

@LaszloLango Sorry, i kind of randomly shot commands to git this morning while being sleepy :D

@LaszloLango
Copy link
Contributor

LaszloLango commented Aug 5, 2016

@sergioamr, no problem, but @akosthekiss landed a patch a few minutes ago, so yours is out of date again.

@sergioamr
Adapted the makefiles to the new build system
67b494d 
- Zephyr now requires two passes to create the configuration for the cross compiling
- Added the missing bits required to build a valid new jerryscript minimal configuration

JerryScript-DCO-1.0-Signed-off-by: Sergio Martinez sergio.martinez.rodriguez@intel.com
@sergioamr sergioamr force-pushed the wip/sergio/zephyr_new_build_system branch from 03c6d8e to 67b494d Compare August 5, 2016 12:12
@LaszloLango LaszloLango merged commit 67b494d into jerryscript-project:master Aug 5, 2016
@pfalcon
Copy link
Contributor

pfalcon commented Aug 5, 2016

@sergioamr: Congrats on merging, now let's brainstorm https://jira.zephyrproject.org/browse/ZEP-644 ;-)

@zherczeg zherczeg mentioned this pull request Mar 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Undesired behaviour jerry-port Related to the port API or the default port implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sergioamr @LaszloLango @pfalcon @robertsipka @zherczeg